Skip to content

Conversation

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Oct 21, 2025

See related microsoft/vcpkg#47735

OK, so here's the situation:

ci.baseline.txt has:

# Missing system libraries
qtwayland:arm64-osx=skip
qtwayland:x64-osx=skip

In microsoft/vcpkg#47735 , qtwayland depends on qtbase[wayland], which implicates the same missing system libraries. This causes qtbase to contain the feature wayland when we try to build it, even though qtwayland is skipped. That makes qtbase fail to build and breaks everything that depends on qt.

The fix is just to not include explicitly skipped ports when we are forming the list of specs we want when building the initial action plan.

======================

As a result of reviewing competing change from @Neumann-A #1821 (comment) which also added more correct skip outputs, this also contains a full overhaul of our console output for the ci command to include more status in the "table" at the beginning, and ensure every port is included in the table at the beginning and in the summary counts at the end.

I think @Neumann-A and @dg0yt should be listed as effective coauthors on this when merging. @Neumann-A suggested the original behavior change and wrote #1821 with an initial shot at it. @dg0yt contributed several helpful test cases.

…icrosoft/vcpkg#47735

OK, so here's the situation:

`ci.baseline.txt` has:

```console
# Missing system libraries
qtwayland:arm64-osx=skip
qtwayland:x64-osx=skip
```

In microsoft/vcpkg#47735, `qtwayland` depends on `qtbase[wayland]`, which implicates the same missing system libraries. This causes `qtbase` to contain the feature `wayland` when we try to build it, even though `qtwayland` is skipped.
@BillyONeal
Copy link
Member Author

Alternative to #1821

@BillyONeal
Copy link
Member Author

BillyONeal commented Oct 21, 2025

Before the fix, the test output is something like:

Detecting compiler hash for triplet x86-windows...
Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.44.35207/bin/Hostx64/x86/cl.exe
            skipped-features:x86-windows:        *: 54830d1c353d4c11b0fe77014a1b7973eba052729c77fc1597c2eb5506df875c
             skipped-depends:x86-windows:     skip: d5aa71e67e593c1c94e5090c89621c2e0d01c0c928149ba77ea38b8b0a18176c
Time to determine pass/fail: 2.8 s
Installing 1/1 skipped-features[core,fail-if-included]:[email protected]...
Building skipped-features[core,fail-if-included]:[email protected]...
CMake Error at C:/Dev/vcpkg-tool/azure-pipelines/e2e-assets/ci-skipped-features/skipped-features/portfile.cmake:3 (message):
  The feature 'fail-if-included' should not be included.
Call Stack (most recent call first):
  scripts/ports.cmake:206 (include)


error: building skipped-features:x86-windows failed with: BUILD_FAILED
See https://learn.microsoft.com/vcpkg/troubleshoot/build-failures?WT.mc_id=vcpkg_inproduct_cli for more information.
Elapsed time to handle skipped-features:x86-windows: 39.1 ms
Total install time: 39.8 ms

Triplet: x86-windows
RESULTS
  skipped-features:x86-windows: BUILD_FAILED: 39.1 ms

SUMMARY FOR x86-windows
  BUILD_FAILED: 1

After:

Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.44.35207/bin/Hostx64/x86/cl.exe
             skipped-depends:x86-windows: skip
            skipped-features:x86-windows:      *: bd8a73d4f54b8505b84fcb4f15268b44c4b7063986ce00a843ff904a0d8f410b
Time to determine pass/fail: 3.8 s
Installing 1/1 skipped-features:[email protected]...
Building skipped-features:[email protected]...
-- Skipping post-build validation due to VCPKG_POLICY_EMPTY_PACKAGE
Elapsed time to handle skipped-features:x86-windows: 61.4 ms
skipped-features:x86-windows package ABI: bd8a73d4f54b8505b84fcb4f15268b44c4b7063986ce00a843ff904a0d8f410b
Total install time: 63.8 ms


Triplet: x86-windows
  skipped-features:x86-windows: SUCCEEDED: 61.4 ms

SUMMARY FOR x86-windows
  SUCCEEDED: 1
  EXCLUDED: 1

Notes:

  • Table at the top is sorted alphabetically now
  • Skipped entries no longer claim to have an ABI hash (they aren't participating in the plan)
  • The skipped port properly is reported as "EXCLUDED" at the end.

@BillyONeal
Copy link
Member Author

Oh hmmm this is suboptimal because the skip isn't in the report at the beginning. Hmmm....

@BillyONeal
Copy link
Member Author

I need to consider / test the impact of known fails here.

@BillyONeal
Copy link
Member Author

https://dev.azure.com/vcpkg/public/_build/results?buildId=121604&view=logs&j=7922e5c4-0103-5f8f-ad17-45ce9bb98e80&t=8ceef4ed-9be5-594f-e707-4e1dd58a3d21

Compiler found: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.44.35207/bin/Hostx64/x64/cl.exe
                         acl:x64-windows:     skip
          aliyun-oss-cpp-sdk:x64-windows:     skip
                        alsa:x64-windows:     skip
                   appstream:x64-windows:  cascade

Example output where we don't print the ABI sha comes from this line:

bool supp = supported_for_triplet(var_provider, provider, spec.package_spec);

@BillyONeal
Copy link
Member Author

@ras0219-msft @vicroms @MahmoudGSaleh @AugP and I discussed today. Rough feedback sounds like:

  • Every port-name needs to be printed somewhere in the output so that people can understand what happened to them.
  • Given that we have existing output that prints cascade or skip with no ABI SHA, we probably should print these exclusions in the same way.
  • Billy needs to check what happens for known fails vs. true skips (CI ABI hashes should not differ from PR ABI hashes)

=================

I also observe that I should probably test the XUnit and summary output for these cases.

@BillyONeal BillyONeal marked this pull request as draft October 23, 2025 22:55
@Neumann-A
Copy link
Contributor

@BillyONeal Do you have an estimate when this is finished since it is blocking the qt pr?

}
}

// FIXME this is the root of the problem: `results` should have *all* of the results, not need to be somehow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix that 'excluded' is not properly accounted for in the build report, and in the xunit output, etc.

}
else
{
result.excluded.emplace_back(PackageSpec{scfl->to_name(), target_triplet});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also calculate exclusions for the host triplet? For example:

qt-complicated:x64-linux=skip

and

foo:arm64-android -> qt-complicated:$host
qt-complicated:$host -> qtbase[problem]:$host

where qtbase[problem]:x64-linux is a problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because this calculating what we morally give to the 'vcpkg install' inside 'ci', and we don't ever add host packages there. Any handling of host dependencies needs to happen in prune_entirely_known_action_branches (formerly known as reduce_action_plan)

CreateInstallPlanOptions create_install_plan_options(
randomizer, host_triplet, UnsupportedPortAction::Warn, UseHeadVersion::No, Editable::No);
randomizer.get(), host_triplet, UnsupportedPortAction::Warn, UseHeadVersion::No, Editable::No);
auto action_plan = compute_full_plan(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there's still an unfortunate result when a user makes a small deviation outside a correct plan. For example, adding a dependency to an excluded package. While we know by construction that the resulting CI plan is "invalid", there's a bad constraint amplification problem where one bad constraint (use the excluded package) gets amplified into all of the excluded package's transitive constraints (that got it excluded in the first place).

Probably not worth solving right now, because it would require the dependency calculator to become aware of exclusions and ignore transitive constraints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know this problem is what we invented =pass for. I'm not sure what you mean by "invalid", that would still be a valid plan, even if it would be empty

…ures

# Conflicts:
#	src/vcpkg/commands.ci.cpp
…ures

# Conflicts:
#	azure-pipelines/end-to-end-tests-dir/ci.ps1
…Cascade

Also want to add some more test cases.
@BillyONeal BillyONeal marked this pull request as ready for review November 6, 2025 03:46
case BuildResult::ExcludedByParent:
case BuildResult::ExcludedByDryRun:
case BuildResult::Unsupported:
case BuildResult::Cached: // should this be "Pass" instead?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a question for reviewers

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to have this match ExcludedByParent. I don't think ExcludedByParent should be Pass, so this LGTM.

.line_break();
}

if (!test.features.empty())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the "we know what features but don't know the ABI tag or vice versa" condition is no longer possible. If we know what features you have, that means we got a plan with you in it, which means we have an ABI hash. Otherwise we don't know either.

throw "feature-not-sup's baseline pass entry should result in a regression because the port is not supported"
}
if (-not ($ErrorOutput.Contains("REGRESSION: dep-on-feature-not-sup:${Triplet} is marked as pass but one dependency is not supported for ${Triplet}."))) {
if (-not ($ErrorOutput.Contains("REGRESSION: dep-on-feature-not-sup:${Triplet} cascaded, but it is required to pass. ("))) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a change in behavior: before, a cascade implied by ci.baseline.txt behaved differently than a "surprise" cascade, the new behavior treats all cascades the same. I think the old behavior was a bug based on the fact that we were implementing how that stuff got reported multiple times and all cascades should be the same for reporting purposes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

The red bits are changed.

Comment on lines 12 to +17
Excluded,
ExcludedByParent,
ExcludedByDryRun,
Unsupported,
CacheMissing,
Cached,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ras0219-msft These are the names you said you wanted to bikeshed, go for it

const CiBaselineData& cidata,
const std::string* cifile,
bool allow_unexpected_passing,
bool is_independent);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE: Removal of the "independent" concept, see discussion #1830

ras0219-msft
ras0219-msft previously approved these changes Nov 12, 2025
Copy link
Collaborator

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

All comments are considerations that would be suitable to investigate in separate PRs.

result_string = "Skip";
break;
case BuildResult::CacheMissing:
case BuildResult::Downloaded:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unfortunate to have so many lines dedicated to an assertion buried deep in the formatting logic, since the code flaw is likely much higher up the stack. No change requested.

{
StringLiteral result_string = "";
switch (test.result)
switch (test.ci_result.code)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This switch is long enough that a table lookup might be clearer. No change requested.

// Do we have 20 yet?
using enum BuildResult;
enum { error, pass, fail, skip };
static uint8_t codemap[] = { [Succeeded] = pass, [BuildFailed] = fail, [PostBuildChecksFailed] = fail, [FileConflicts] = fail, ... };

if (static_cast<int>(test.ci_result.code) > sizeof(codemap)) abort();

};

struct UnknownCIPortsResults
struct CIPreBuildStatus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type mixes logic results (known) with formatting results (report_lines / abis); it might be cleaner to reduce this to just std::map<PackageSpec, BuildResult> known; and have the report code deal with calculating report_lines / abis on the fly from the action_plan and/or excluded.

Request no change; consider follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and have the report code deal with calculating report_lines / abis on the fly from the action_plan and/or excluded

I tried to do this. Unfortunately that forces recalculating a bunch of information we have in the for loop on line 125; I considered mixing the formatting concern the lesser evil than duplicating that control flow.

void reduce_action_plan(ActionPlan& action_plan,
const std::map<PackageSpec, BuildResult>& known,
View<std::string> parent_hashes)
void prune_entirely_known_action_branches(ActionPlan& action_plan, const std::map<PackageSpec, BuildResult>& known)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since known now must contain every spec in the action plan, should it be a simpler std::unique_ptr<BuildResult[]> or vector?

No changes requested.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

known now must contain every spec in the action plan

known contains things not in the action plan as well which would make that somewhat painful

format_ci_result(r.first, r.second, cidata, ci_baseline_file_name, allow_unexpected_passing, true);
if (!msg.empty())
if (scfl->source_control_file->has_qualified_dependencies() ||
!scfl->source_control_file->core_paragraph->supports_expression.is_empty())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably check features as well for qualified dependencies and supports expressions.

But I think this is just a performance optimization, not correctness.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but I didn't actually change this so I'm going to avoid changing it. (I just extracted this block as a function:

std::vector<PackageSpec> packages_with_qualified_deps;
for (auto&& spec : specs)
{
auto&& scfl = provider.get_control_file(spec.package_spec.name()).value_or_exit(VCPKG_LINE_INFO);
if (scfl.source_control_file->has_qualified_dependencies() ||
!scfl.source_control_file->core_paragraph->supports_expression.is_empty())
{
packages_with_qualified_deps.push_back(spec.package_spec);
}
}

)

Comment on lines 312 to 320
const TripletExclusions* target_triplet_exclusions = nullptr;
for (auto&& te : exclusions_map.triplets)
{
msg::write_unlocalized_text_to_stderr(Color::none, output);
if (te.triplet == target_triplet)
{
target_triplet_exclusions = &te;
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const TripletExclusions* target_triplet_exclusions = nullptr;
for (auto&& te : exclusions_map.triplets)
{
msg::write_unlocalized_text_to_stderr(Color::none, output);
if (te.triplet == target_triplet)
{
target_triplet_exclusions = &te;
break;
}
}
auto it = Util::find_if(exclusion_map.triplets, [&](auto& x) { return x.triplet == target_triplet; });
const TripletExclusions* target_triplet_exclusions = it != exclusion_map.triplets.end() ? &*it : nullptr;

Could also simplify target_triplet_exclusions to:

const SortedVector<std::string>* exclusions;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that much shorter to do this and it involves lambdas which is a tiebreaker-against for me but I don't care THAT much so I applied it.

Comment on lines 448 to 456
const auto parsed_object =
Json::parse(parent_hashes_text.content, parent_hashes_text.origin).value_or_exit(VCPKG_LINE_INFO);
const auto& parent_hashes_array = parsed_object.value.array(VCPKG_LINE_INFO);
for (const Json::Value& array_value : parent_hashes_array)
{
auto abi = array_value.object(VCPKG_LINE_INFO).get(JsonIdAbi);
Checks::check_exit(VCPKG_LINE_INFO, abi);
parent_hashes.insert(abi->string(VCPKG_LINE_INFO).to_string());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to extract this as a function:

ExpectedL<std::unordered_set<std::string>> parse_parent_hashes(const std::string& text_content, int origin);

@BillyONeal BillyONeal merged commit 0ebc9cd into microsoft:main Nov 13, 2025
9 of 13 checks passed
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Nov 17, 2025
Billy made a bit of a mistake in microsoft#1822 resulting in the blizzard of failures in https://dev.azure.com/vcpkg/public/_build/results?buildId=122788
@BillyONeal BillyONeal mentioned this pull request Nov 17, 2025
BillyONeal added a commit that referenced this pull request Nov 17, 2025
Billy made a bit of a mistake in #1822 resulting in the blizzard of failures in https://dev.azure.com/vcpkg/public/_build/results?buildId=122788
@BillyONeal BillyONeal deleted the ci-skip-baseline-features branch November 25, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants